Skip to content

Conversation

@lance5890
Copy link
Contributor

@lance5890 lance5890 commented Aug 18, 2025

  1. I noticed that @wking has tried to remove the automountServiceAccountToken in Revert "prevent pod deployment deadlock due to custom SA projected volume injection" #607 when upgrade to 4.9(this is not suppuorted when upgrade, because cvo do not suppuort remvoing the automountServiceAccountToken), and finally revert this in Bug 2005581: install/0000_00_cluster-version-operator_03_deployment: Explicit kube-api-access #660

  2. removing the automountServiceAccountToken not only needed by cvo, but also by OCPBUGS-60496: Use BoundServceAccountTokenVolume dy default cluster-kube-apiserver-operator#1896, and OCPBUGS-60568: Use BoundServceAccountTokenVolume dy default cluster-kube-controller-manager-operator#858, and OCPBUGS-60572: Use BoundServceAccountTokenVolume dy default cluster-kube-scheduler-operator#571, and Use BoundServceAccountTokenVolume dy default library-go#2001

  3. removing the automountServiceAccountToken is ok when install a new cluster, but will encounter a error: error creating clients: invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable issue because the cvo do not suppuort setting the new automountServiceAccountToken value (which will use the existing false value in the existing obj)

  4. maybe we should merge this PR first ,and then the related cluster operators

  5. and finally remove the automountServiceAccountToken for cvo in the next upgrade version

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 18, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lance5890
Once this PR has been reviewed and has the lgtm label, please assign davidhurta for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 18, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 18, 2025

Hi @lance5890. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@lance5890 lance5890 changed the title Suppuort automountServiceAccountToken modify OCPBUGS-60579: Suppuort automountServiceAccountToken modify Aug 18, 2025
@openshift-ci-robot openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Aug 18, 2025
@openshift-ci-robot
Copy link
Contributor

@lance5890: This pull request references Jira Issue OCPBUGS-60579, which is invalid:

  • expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Aug 18, 2025
@lance5890
Copy link
Contributor Author

/cc @vrutkovs @wking

@vrutkovs
Copy link

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 18, 2025
@lance5890 lance5890 force-pushed the suppuort_automountServiceAccountToken branch from 9441c41 to baeeb80 Compare August 18, 2025 07:09
@lance5890 lance5890 force-pushed the suppuort_automountServiceAccountToken branch from baeeb80 to 9f94bdc Compare August 18, 2025 07:42
@lance5890
Copy link
Contributor Author

/retest

@lance5890
Copy link
Contributor Author

/retest-required

2 similar comments
@lance5890
Copy link
Contributor Author

/retest-required

@lance5890
Copy link
Contributor Author

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 19, 2025

@lance5890: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-scos-e2e-aws-ovn 9f94bdc link false /test okd-scos-e2e-aws-ovn

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-app: cluster-version-operator
spec:
automountServiceAccountToken: false
automountServiceAccountToken: false # removed in the next version.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kube-api-access mount point should also be removed in the next version

@tchap
Copy link
Contributor

tchap commented Nov 26, 2025

Taking over this change in #1264

/close

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2025
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot closed this Nov 26, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 26, 2025

@tchap: Closed this PR.

In response to this:

Taking over this change in #1264

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci-robot
Copy link
Contributor

@lance5890: This pull request references Jira Issue OCPBUGS-60579. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

  1. I noticed that @wking has tried to remove the automountServiceAccountToken in Revert "prevent pod deployment deadlock due to custom SA projected volume injection" #607 when upgrade to 4.9(this is not suppuorted when upgrade, because cvo do not suppuort remvoing the automountServiceAccountToken), and finally revert this in Bug 2005581: install/0000_00_cluster-version-operator_03_deployment: Explicit kube-api-access #660

  2. removing the automountServiceAccountToken not only needed by cvo, but also by OCPBUGS-60496: Use BoundServceAccountTokenVolume dy default cluster-kube-apiserver-operator#1896, and OCPBUGS-60568: Use BoundServceAccountTokenVolume dy default cluster-kube-controller-manager-operator#858, and OCPBUGS-60572: Use BoundServceAccountTokenVolume dy default cluster-kube-scheduler-operator#571, and Use BoundServceAccountTokenVolume dy default library-go#2001

  3. removing the automountServiceAccountToken is ok when install a new cluster, but will encounter a error: error creating clients: invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable issue because the cvo do not suppuort setting the new automountServiceAccountToken value (which will use the existing false value in the existing obj)

  4. maybe we should merge this PR first ,and then the related cluster operators

  5. and finally remove the automountServiceAccountToken for cvo in the next upgrade version

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai
Copy link

coderabbitai bot commented Nov 26, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Adds support for merging AutomountServiceAccountToken in PodSpec resource reconciliation. Implementation includes the merge logic in core.go, corresponding test cases in core_test.go, and inline comments in manifest files documenting future removal.

Changes

Cohort / File(s) Summary
Manifest configuration
install/0000_00_cluster-version-operator_03_deployment.yaml, pkg/payload/testdata/TestRenderManifest_expected_cvo_deployment.yaml
Added inline comment "# removed in the next version." to automountServiceAccountToken: false without functional changes.
Core merge logic
lib/resourcemerge/core.go
Added handling for AutomountServiceAccountToken via setBoolPtr to merge required.AutomountServiceAccountToken into existing.AutomountServiceAccountToken alongside other PodSpec fields.
Merge logic tests
lib/resourcemerge/core_test.go
Added three new test cases to TestEnsurePodSpec covering AutomountServiceAccountToken scenarios: none state (existing false, input empty), changed state (existing true, input false), and no-changes state (existing false, input false).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that setBoolPtr correctly handles nil pointer logic for AutomountServiceAccountToken
  • Confirm test cases align with the expected merge behavior pattern used for similar PodSpec fields (e.g., RestartPolicy, HostNetwork)
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9de00ba and 9f94bdc.

📒 Files selected for processing (4)
  • install/0000_00_cluster-version-operator_03_deployment.yaml (1 hunks)
  • lib/resourcemerge/core.go (1 hunks)
  • lib/resourcemerge/core_test.go (1 hunks)
  • pkg/payload/testdata/TestRenderManifest_expected_cvo_deployment.yaml (1 hunks)

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants